fix(store): snapshot() and store materialization keep symbol-keyed props#2895
Merged
Merged
Conversation
🦋 Changeset detectedLatest commit: 7cb16a5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Merging this PR will regress 1 benchmark
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing |
after writes (closes solidjs#2894)
…elpers The StoreSetter Mutable draft type and the createStore overload narrowing were not needed by the fix or its tests (typecheck passes without them); keep the PR scoped to the symbol-key materialization bug. Fold the override-merge loops of getKeysImpl/getStoreSymbols into a shared mergeOverrideKeys and build ownEnumerableKeysPlain from ownEnumerableSymbols (-220 B min / -36 B gzip). Co-authored-by: Cursor <cursoragent@cursor.com>
ryansolid
force-pushed
the
fix/snapshot-symbol-keys
branch
from
July 17, 2026 15:36
85be00c to
7cb16a5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2894
Summary
After any write to a store subtree,
snapshot()of it drops symbol-keyed properties — and writing that subtree into another store drops them too. Before the first write they survive by accident (the identity-return fast path hands back the original object), so the loss only appears once the object has an override, and initial tests against untouched records pass.This is the remaining materialization variant of #2769: that fix covered symbol keys in the set trap,
storeSetter,storePath, andmerge/omitviaownEnumerableKeys, and #2851 coveredreconcile()'s diff loops — but the internal materialization paths still enumerated withObject.keys.Root cause
getKeys()seeds its base key list fromObject.keys(source)(strings only) and drives:snapshotImpl—snapshot()/deep()of any written record iterate string keys into the eagerly-created copy, so symbol props never land; and on the no-override path, symbol-keyed store-in-store values are never visited, so they leak wrapped instead of unwrapping.unwrapStoreValue— the setter's store-to-store materialization, so copying a previously-written subtree into another store drops its symbol props.walkAffectsScope— a keylessaffects(store)never descends into symbol-keyed subtrees, so their nodes miss pending coverage.Note the boundary: symbols living in the override layer (added after creation) were already enumerated (
Reflect.ownKeys(override)in the merge loop) — the gap was specifically base-object symbols, which is why the failure needs a prior write to appear.Fix
New symbol-aware enumeration helpers, applied at exactly those three sites; the public
getKeysis unchanged, soreconcile'sgetAllKeysand theownKeystrap keep their existing paths:getStoreKeys— enumerable strings + enumerable symbols, honoring override adds and$DELETEDremovals. Plain objects stay on theObject.keysfast path and only pay onegetOwnPropertySymbolscall (ownEnumerableKeysPlain); wrapped store-in-store sources keep the single-passownEnumerableKeysso theirownKeystrap fires once, untracked.getStoreSymbols— symbol-only variant for the array branches, so large index lists aren't allocated and scanned a second time; both array walks (snapshot and affects) get a separate symbol pass after the numeric loop.lookupis set), sosnapshot()of a plain non-store object keeps its old string-only behavior. Non-enumerable symbols are filtered, so internal slots can never leak into copies (test-pinned). Symbol-keyed accessors are skipped, matching the string-key branches.Tests
Seventeen new tests (ten confirmed failing red-first on the unfixed source; seven are boundary-semantics controls):
tests/snapshot.test.ts— symbol keys survive after a write; a write inside a symbol-keyed subtree is captured; unread and written-parent symbol-keyed store-in-store values unwrap; array symbol metadata survives an index write; store-to-store materialization keeps symbols; shared references between symbol and string keys stay aliased in the copy; cycles through symbol keys preserve identity; deleted symbol keys (object and array) stay deleted; non-enumerable symbols stay excluded from copies; a symbol key added after a prior snapshot appears; untouched nested values keep symbolstests/store/utilities.test.ts—deep()tracks updates in a symbol-keyed subtree, and notifies on symbol key addition and deletiontests/question-scoped-pending.test.ts— keylessaffects(store)covers an untouched symbol-keyed grandchild, and symbol-keyed array metadataFull solid-signals suite: the diff versus pristine is exactly the intended flips, nothing else — including the #2893 affects-audit tests, which exercise the same subsystem as the
walkAffectsScopechange.solid-jsand@solidjs/webrebuilt against the fixed package and re-run at their baselines; source passes the typecheck. Verified atnext@d823a806(2.0.0-beta.19).Performance
Micro-benched with symbol-free fixtures (the common case): no measurable change for written arrays, store-to-store writes, wide objects (~3%), or the existing
reconcile/deep()benches. Snapshotting a deep tree of many small objects pays ~14% — onegetOwnPropertySymbolscall per visited object, which is the minimum price of seeing symbol keys at all.snapshot()is a serialization-boundary operation, not a per-read hot path. If this matters, a follow-up could gate enumeration behind aWeakSetmarker like #2851'ssymbolKeyedRecords.Does this exist in Solid 1.x?
Regression. Verified against 1.9.14:
unwrapafter a write keeps symbol keys, and copying a written subtree into another store keeps them.Full disclosure: I wrote this with the help of an AI assistant (Claude Fable 5) and reviewed every line before pushing. All red-first tests were confirmed failing on the unfixed code.